Size Change: +6.79 kB (+1%)

Total Size: 1.04 MB

_{compressed-size-action}

This is really, really nice:

The above is a really basic navigation pattern that I think will be popular — Site Title would work well there also.

I used space-between to accomplish it, and realized that there's some margin work to do (or better yet, move to using gap, #32367), and I pushed a small space-between fix that appears to have regressed in trunk as well (happy to push it separately there as well, if need be). But both those are in the category of "trivial", and the important part is that the idea of allowing these two blocks feels very validated by this PR. Very nice work, thank you.

I like this PR a lot, and it's very simple and neat. Revisiting it now, the primary thing stands out to me is the lack of margin:

The navigation block has CSS baked in that applies default margins to menu items inside. This:

.wp-block-page-list, .wp-block-page-list > .wp-block-pages-list__item, .wp-block-navigation__container > .wp-block-navigation-link {
    margin: 0 2em 0 0;
}

While we could duplicate that CSS or expand it to also target this block, the problem is that there are many compounding edgecases for those margins making it non trivial to duplicate, and at very high risk of drifting:

• The margin is smaller if the navigation block has a background (which gives menu items padding instead)
• The margin is different for items in dropdowns
• The margin is different for items in the hamburger menu
• The margin is different for the vertical navigation item

The CSS is already fairly gnarly, and this exact problem already affects the Home link item:

The current best suggestion for how to address this is #33048, which is to add more generic "menu item" CSS classes that can be targeted by the margin and positioning rules, so that we don't have to duplicate and expand the complexity for every new menu item added.

I wouldn't necessarily object to this PR landing as-is, without addressing #33048 first. But it does seem slighly more urgently in need of being addressed. I'd love to pair with someone on fixing that, if anyone has bandwidth.

I wouldn't necessarily object to this PR landing as-is, without addressing #33048 first. But it does seem slighly more urgently in need of being addressed. I'd love to pair with someone on fixing that, if anyone has bandwidth.

As commented here, I'm happy to work on it as the problem does need to be addressed soon. The changes themselves should be straightforward, once we agree on the classnames.

@jasmussen with #33918 merged, what's the best way forward here? Should Site Title and Logo conditionally display the wp-block-navigation-item classname when they are inside the Nav block?

If so, I'm thinking we should have the CSS cleaned up and using the new classnames before we do that, so it's easier to test how it works.

Should Site Title and Logo conditionally display the wp-block-navigation-item classname when they are inside the Nav block?

In my dream scenario, the navigation block itself would apply classnames to its children based on their location inside. Would that be possible at all?

If so, I'm thinking we should have the CSS cleaned up and using the new classnames before we do that, so it's easier to test how it works.

I'm spread across a few separate projects at the moment, but I will get to the CSS cleanup as soon as possible! Very excited about it, thanks again.

In my dream scenario, the navigation block itself would apply classnames to its children based on their location inside. Would that be possible at all?

It's possible to set classnames conditional on position via block attributes, but I need to look into what happens when the blocks are moved around inside navigation. For this to work, the parent block will need to be aware of changes in order of its inner blocks.

What classnames would you like to apply to which locations?

What classnames would you like to apply to which locations?

It's mostly a philosophy that the layout of child elements should be handled by the container if possible. In this case, due to the fact that we allow a variety of elements inside the navigation block (and that it can vary from editor to frontend), ideally we could just have a single generic CSS class that indicates: this is now a menu item. So it could be the same CSS class as you applied in that separate classname PR.

What do you think?

It's mostly a philosophy that the layout of child elements should be handled by the container if possible.

How is this being handled in other cases? I know there was some work done recently on the Group block; have there been any thoughts around adding classnames to child blocks? If so it would be best to try for a solution that works everywhere.

Either way, I'm wondering if the context solution @getdave and I were discussing on the generic classnames PR might be a good fit. If it turns out that Site Title and Logo might need different custom classnames when they are children of some other block, passing a classname though context is a good solution. Even if they only need a classname when they're inside Navigation, we'll have to leverage some form of context to know when the blocks are inside Navigation.

(None of that should change the work done in the previous PR though: Page List needs classnames added in several places in its markup, which would be unwieldy to pass through context.)

How is this being handled in other cases? I know there was some work done recently on the Group block; have there been any thoughts around adding classnames to child blocks? If so it would be best to try for a solution that works everywhere.

No, I think adding child classes is a bit of an edgecase for the navigation block due to its complex layout and structure. I was thinking most in terms of layout and such, where the gap property can be assigned to the container, and thereby affecting the layout of child items directly.

@jasmussen I've updated this PR to add the wp-block-navigation-item classname to the li that wrap the site title and logo blocks. They are only added on the server side though, so we may still need some adjustment to styles in the editor. Once both this PR and your CSS one are merged, we can see how they work together and determine if anything else is needed.

I guess this is now ready for a final review!

Wonderful. I landed the CSS refactor (thanks again for the reviews), so a rebase on this one might unlock things.